Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Subset in request's collect #39191

Merged
merged 4 commits into from
Oct 15, 2021
Merged

[8.x] Subset in request's collect #39191

merged 4 commits into from
Oct 15, 2021

Conversation

iraldoad
Copy link
Contributor

@iraldoad iraldoad commented Oct 13, 2021

This adds the ability to get a subset in request collect function.

Before

    public function collect($key = null)
    {
        return collect($this->input($key));
    }

After

    public function collect($key = null)
    {
        if (is_array($key))
            return collect($this->only($key));
        else
            return collect($this->input($key));
    }

fix #39190

@iraldoad iraldoad changed the title feat: now the request collection option allows a subset of inputs Subset in request collect Oct 13, 2021
@iraldoad iraldoad changed the title Subset in request collect Subset in request's collect Oct 13, 2021
Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Left a comment.

return collect($this->input($key));
if (is_array($key)) {
return collect($this->only($key));
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don;t use else after a return

@GrahamCampbell GrahamCampbell changed the title Subset in request's collect [8.x] Subset in request's collect Oct 13, 2021
@BrandonSurowiec
Copy link
Contributor

BrandonSurowiec commented Oct 14, 2021

It would be a good idea to add some test coverage. You can see the previous commit that added this method to give you an idea of where to add the test: iraldoad@4cb7660#diff-50be1df2e396478cd5abf036e618ba2a6c09c5dfcb6429d172faea92dae1c904

You could make this method a one-liner:

    public function collect($key = null)
    {
        return collect(is_array($key) ? $this->only($key) : $this->input($key));
    }

@taylorotwell taylorotwell marked this pull request as draft October 14, 2021 13:12
@taylorotwell
Copy link
Member

Draft pending test coverage.

@iraldoad iraldoad marked this pull request as ready for review October 14, 2021 20:09
@iraldoad
Copy link
Contributor Author

Working on it.

$this->assertInstanceOf(Collection::class, $request->collect(['users']));
$this->assertTrue($request->collect(['developers'])->isEmpty());
$this->assertTrue($request->collect(['roles'])->isNotEmpty());
$this->assertEquals(['roles' => [4, 5, 6]], $request->collect(['roles'])->all());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're only ever passing one key into the collect() method on your test. It would be better to pass multiple keys in to really test that we're able to grab more than one key with your addition of only(). Expand your initial request to 3-4 parameters.

- more request parameters are added
- more tests are added with different keys
@taylorotwell taylorotwell merged commit 72aeac2 into laravel:8.x Oct 15, 2021
@BrandonSurowiec
Copy link
Contributor

@iraldoad Great improvement, congrats on the merge! 🥳

@iraldoad
Copy link
Contributor Author

iraldoad commented Oct 16, 2021

@iraldoad Great improvement, congrats on the merge! 🥳

Thank you very much for the suggestions and reviews. Happy to contribute to this great project! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request collect with only
4 participants